Transmute Kernels to add OMP where not already - fix existing functionality#607
Transmute Kernels to add OMP where not already - fix existing functionality#607Benjamin Went (MetBenjaminWent) wants to merge 39 commits into
Conversation
…rides for mphys_kernel_mod to use the correct one
…ns list. Possibly a wider options fix needs to be rolled in with this ticket......
…for now, overrides should be used to move
…dynamic over top k loop
…ions. Depricate old methods, and better align script with existing methods
…tions by accident, needed for raising jules_exp_kernel_mod into local. Also needed pure call option which is a minor extra, and makes the script more flexible
|
Some files are running into this error: #613 has been raised to look at this again when the build system can accommodate it. |
|
Issue with CCE build time is cropping up sadly. The problem was a bug/feature in psyclone where it changed a case statement into nested if statements instead of using if-else-if, which really upsets the compiler. Both the files you've mentioned have giant case statements, so I think you're probably right. Work here for Fix to PSyclone in their Issue here: stfc/PSyclone#3418 |
Iva Kavcic (TeranIvy)
left a comment
There was a problem hiding this comment.
Thanks for the heads-up on approval/review, Benjamin Went (@MetBenjaminWent). I have two requests/questions in the modified transformation scripts (see inline):
- There is no year in the copyright in some of the scripts, and in some is. What is the standard on this?
- The input argument
psyirand the return/result of the functiontransare not documented, and it would be useful for the understanding of functionality.
Iva Kavcic (TeranIvy)
left a comment
There was a problem hiding this comment.
Thanks for making changes. I'm afraid I misunderstood documentation of return in the trans functions. PSyclone convention only does it in internal classes/functions. I see they're empty in your changes so I'll just ask for the deletion of empty returns. Otherwise it's all well.
Iva Kavcic (TeranIvy)
left a comment
There was a problem hiding this comment.
All good now.
PR Summary
As noted in the umbrella issue, not all of the kernels have OMP coverage, which means we are loosing some performance at higher thread counts, even if it's smaller a smaller runtime cost than some other sections.
Also with the addition of a local script for kernels, we need to move the
SCRIPT_OPTIONS_DICTformphys_kernel_modfrom the global area to the kernels.With the move to the head of PSyclone, 3.3.1, it seems that the
optionsparameter passed to a transformation, no longer passes everything through, such asignore_dependencies_for. I am aware that STFC are trying to deprecate the options dict in favor of explicit by optional parameters to all transformations. We can catch modifications in this PR.As I'm already fixing some piping in the scripts, where sensible there is some housekeeping, and lifting duplicate scripts into the local script.
This PR Contains:
mphys_turb_gen_kernel_mod,lsp_prognostic_tnuc_kernel_mod,lw_rad_tile_kernel_mod(Note RESOLVE_IMPORTS errors with PSyclone Transmute #613),bm_kernel_mod,smith_kernel_mod,spectral_gwd_kernel_mod,photol_kernel_mod,tracer_mix_kernel_mod,SCRIPT_OPTIONS_DICTformphys_kernel_modto kernel - Moved up into Enable Transmute script for bl imp and exp kernels #361local.pyscript for PSyclone 3.3.1. - Part moved up into Enable Transmute script for bl imp and exp kernels #361, part here.global.pyscript for PSyclone 3.3.1.pc2_conv_coupling_kernel_mod.pyfor PSyclone 3.3.1.lw_kernel_modandsw_kernel_modfor PSyclone 3.3.1.conv_gr_kernel_modfor PSyclone 3.3.1. - Moved into update Transmute for conv_gr_kernel_mod #583bm_tau_kernel_modto use thelocal.pyscript as scripts are effectively identical.jules_extra_kernel_modto use thelocal.pyscript as scripts are effectively identical.pc2_initiation_kernel_modto use thelocal.pyscript as scripts are effectively identical.sw_rad_tile_kernel_modto use thelocal.pyscript as scripts are effectively identical.jules_exp_kernel_modto use thelocal.pyscript. Add checks for OMP ancestory for general safety, and option ofpurecall overrides tolocal.pyscript.jules_imp_kernel_modto use thelocal.pyscript as scripts are effectively identical.CCE Issues currently with, but have still been captured in this ticket to be ready to be activated as needed :
aerosol_ukca_kernel_mod,conv_comorph_kernel_modSci/Tech Reviewer:
Code Reviewer: Joseph Abram (@J-J-Abram)
Issue: #608
Umbrella: #424
Code Quality Checklist
Testing
trac.log
Test Suite Results - lfric_apps - add_omp_more_kernels/run3
Suite Information
Task Information
✅ succeeded tasks - 51
Test Suite Results - lfric_apps - add_omp_more_kernels/run4
Suite Information
Task Information
✅ succeeded tasks - 1592
Security Considerations
Performance Impact
AI Assistance and Attribution
Documentation
PSyclone Approval
Sci/Tech Review
(Please alert the code reviewer via a tag when you have approved the SR)
Code Review